Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

remove power of two check #78

Merged
merged 14 commits into from
Apr 4, 2023
Merged

Conversation

jdonszelmann
Copy link
Collaborator

@jdonszelmann jdonszelmann commented Mar 31, 2023

Replaces #76 (for at least the power of two check), addressing the last commments on #61 by @Sjord. They are completely right that the power of two requirements is actually not really necessary for the const generic ringbuffer as its compiled to the same bitshift anyway

Closes #61

@jdonszelmann jdonszelmann force-pushed the remove-power-of-two-requirement branch from 64803e6 to 8f79270 Compare April 1, 2023 09:17
@jdonszelmann
Copy link
Collaborator Author

@Sjord is this how you'd like it?

@NULLx76
Copy link
Owner

NULLx76 commented Apr 1, 2023

I see MIRI isn't able to be ran on the compile tests, we should probably add an exclusion for that. Also tarpaulin seems broken for some reason, maybe we need to update that pipeline stage

@NULLx76
Copy link
Owner

NULLx76 commented Apr 1, 2023

The cargo tarpaulin action seems to be quite outdated anyways:

So I want to switch to another system for coverage, but I'll give that its own PR.

@jdonszelmann
Copy link
Collaborator Author

Also enabled non-power-of-two alloc ringbuffers. The ability to have non-power-of-two alloc ringbuffers has to be enabled at compile time as benchmarks show that it can be 2-3 times slower than using ands. By default, alloc ringbuffers still use have the power of two requirement

As well as that, made the unchecked capacity version of the alloc ringbuffer unsafe. This is a breaking change. However, using it with a non-power-of-two capacity was literally undefined behavior, so I think we want this to be marked unsafe.

@Sjord
Copy link

Sjord commented Apr 1, 2023

@Sjord is this how you'd like it?

Yes, this makes sense. My opinion shouldn't weigh too heavily though. I used this crate for Advent of Code day 6. The assignment was to find 4 consecutive different characters, and I used this ringbuffer for that. Then, part 2 of the assignment was to find 14 consecutive different characters, and then I ran into problems that only powers of two were supported. I think it makes sense to support any size, as long as users are aware of the performance consequences. But my use-case was artificial and one-off, so I don't want to steer the direction of this crate too much.

Also enabled non-power-of-two alloc ringbuffers. The ability to have non-power-of-two alloc ringbuffers has to be enabled at compile time as benchmarks show that it can be 2-3 times slower than using ands.

There are also crates that speed up the modulo by doing some calculations in advance, which can be useful to speed up alloced ringbuffers of arbitrary size:

src/with_alloc.rs Outdated Show resolved Hide resolved
@jdonszelmann jdonszelmann force-pushed the remove-power-of-two-requirement branch from e195148 to 8fcbb12 Compare April 1, 2023 15:05
@jdonszelmann jdonszelmann requested a review from thedmm April 1, 2023 15:24
@thedmm
Copy link

thedmm commented Apr 1, 2023

It's also possible to modify Alloc to use Box instead of Vec (thereby saving on a usize of storage), but I think that falls outside the scope of this PR. Perhaps something for later.

@jdonszelmann
Copy link
Collaborator Author

jdonszelmann commented Apr 1, 2023

indeed outside of scope.

Would save two words even though

benches/bench.rs Outdated Show resolved Hide resolved
benches/bench.rs Outdated Show resolved Hide resolved
src/with_alloc.rs Outdated Show resolved Hide resolved
src/with_alloc.rs Show resolved Hide resolved
src/with_alloc.rs Outdated Show resolved Hide resolved
@jdonszelmann jdonszelmann requested a review from NULLx76 April 3, 2023 10:07
…ringbuffer into remove-power-of-two-requirement
@jdonszelmann jdonszelmann merged commit dfa9add into master Apr 4, 2023
@jdonszelmann jdonszelmann deleted the remove-power-of-two-requirement branch April 4, 2023 08:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make capacity power of Two check constant for ConstGenericRingBuffer
4 participants